Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[DDW-245] Integrate react-polymorph render props in Daedalus #950

Merged
merged 54 commits into from
Jul 23, 2018
Merged

[DDW-245] Integrate react-polymorph render props in Daedalus #950

merged 54 commits into from
Jul 23, 2018

Conversation

MarcusHurney
Copy link
Contributor

@MarcusHurney MarcusHurney commented May 31, 2018 •
edited
Loading

This PR integrates the develop branch of react-polymorph into Daedalus.

Todo:

  • release new react-polymorph version on npm
  • set correct react-polymorph version in package.json after npm release
  • Replace react-css-themr ThemeProvider with react-polymorph ThemeProvider
  • Change import statements of react-polymorph components, skins, and themes to match new file structure
  • Pass skins to react-polymorph components as a variable created by import statements instead of passing a skin component instance (i.e. doesn't pass skin components with tags<>)
  • general: focus-on-label-click feature in react-polymorph FormField component
  • general: clearing of wallet restore mnemonics when leaving the dialog
  • general: select arrow has inverted color on active/inactive states
    react-polymorph PR 68
  • general: select checkmark should be white in dark theme

screen shot 2018-07-02 at 10 01 12 pm

  • general: select not closing on enter key selection
  • settings screen: disabled state on custom fields (inline editing inputs) is wrong (should look like enabled field)

screen shot 2018-07-02 at 10 03 21 pm

  • ada-redemption screen: isOpeningUpwards not working correctly on select and autocomplete components

screen shot 2018-07-02 at 9 56 30 pm

screen shot 2018-07-02 at 7 10 23 pm

  • create-dialog: mnemonic entry buttons should not have outline if they are disabled (even if they are focused!)

screen shot 2018-07-02 at 10 02 44 pm

  • wallet send screen: in case you enter a valid amount and then type some non-numeric keys the button becomes disabled

Review Checklist:

Basics

  • PR is updated to the most recent version of target branch (and there are no conflicts)
  • PR has good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub
  • Automated tests: All acceptance tests are passing (npm run test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (npm run dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (npm run package / CI builds)
  • There are no flow errors or warnings (npm run flow:test)
  • There are no lint errors or warnings (npm run lint)
  • [N/A] No text changes -> Text changes are proofread and approved (Jane Wild)
  • There are no missing translations (running npm run manage:translations produces no changes)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (npm run storybook)
  • In case of npm dependency changes both package-lock.json and yarn.lock files are updated

Code Quality

  • Important parts of the code are properly documented and commented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-rendering
  • Any code that only works in Electron is neatly separated from components

Testing

  • New feature / change is covered by acceptance tests
  • All existing acceptance tests are still up-to-date
  • New feature / change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review:

  • Merge PR
  • Delete source branch
  • Move ticket to done on the Youtrack board

MarcusHurney added 10 commits May 29, 2018 15:41
@MarcusHurney
[DDW-245] Changes import syntax and paths for react-polymorph compone…
6256b5d 
…nts to use new API
@MarcusHurney
[DDW-245] Replaces react-css-themr ThemeProvider with react-polymorph…
1956f28 
… ThemeProvider
@MarcusHurney
[DDW-245] Removes react-css-themr from dependencies
e396312
@MarcusHurney
[DDW-245] Removes FormField wrapping Input components. Adds themeId t…
da37ea0 
…o Checkbox component.
@MarcusHurney
[DDW-245] Removes react-css-themr dependency
cbb07ae
@MarcusHurney
[DDW-245] Replaces react-css-themr ThemeProvider with react-polymorph's
a366c07
@MarcusHurney
[DDW-245] Removes FormField rendering Inputs. Passes label directly t…
27058fa 
…o Inputs.
@MarcusHurney
[DDW-245] Implements new react-polymorph API in import statements and…
6a5a809 
… components' skin prop
@MarcusHurney
Merge branch 'develop' of github.com:input-output-hk/daedalus into ch…
efe890b 
…ore/ddw-245-integrate-react-polymorph-render-props-in-Daedalus

[DDW-245] Pulls latest from develop branch
@MarcusHurney
[DDW-245] Corrects name of label prop for displaying wallet name
4604ca4
@MarcusHurney MarcusHurney self-assigned this May 31, 2018
DominikGuzei
DominikGuzei previously approved these changes Jun 2, 2018
View reviewed changes
Copy link
Member

@DominikGuzei DominikGuzei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliant work @MarcusHurney! Thanks for doing all the hard work to make my dreams for react-polymorph true 😉 specially love how this actually reduces our lines of code a bit while featuring a better component api (e.g: passing the skins directly, instead of jsx components)

Since this is a huge change, i would love to have one other approval before merging! (@nikolaglumac ?)

@nikolaglumac
Copy link
Contributor

@DominikGuzei sure - I will review this on Monday. Still, we must be very careful due to all of the changes - especially in the NumericInput on the Send screen, special inputs on Wallet Settings screen, etc.

@DominikGuzei would you be willing to sort out the 2 of remaining Todos? (publishing new React-Polymorph version + updating that version in package.json on Daedalus side?)

@nikolaglumac
Copy link
Contributor

This PR should not be merged before the 1.3 release cut-off.

@MarcusHurney
Copy link
Contributor Author

MarcusHurney commented Jun 12, 2018 •
edited
Loading

@DominikGuzei
Thanks for the kind words. I share your dreams for react-polymorph and I highly doubt I would have been able to implement any of this without all the ground work you did. You conceptualized the skyscraper, built the foundation and structure and all the stuff that makes it a monster feat. I just put in the elevators and windows. I'm excited to see where this thing goes and I'm kind of amazed how much cooler stuff can be when people work together. It's nice to feel like something is my baby, but I'd rather see the baby become a spacecraft rather than a kite with my name on it. haha

@nikolaglumac nikolaglumac changed the title [DDW-245] integrate react polymorph render props in daedalus [DDW-245] Integrate React-Polymorph render props in Daedalus Jun 15, 2018
@nikolaglumac
Copy link
Contributor

Hi @MarcusHurney @DominikGuzei - great work!
Almost everything is fixed. I have found some more defects which we need to fix:

  • duplicated styles (Dominik)
  • error state on textarea (Dominik)
  • checkbox component 1px centering issue (Dominik)
  • checkbox disabled state border color in Dark blue theme (Nikola and Saša)

@DominikGuzei DominikGuzei changed the title [DDW-245] Integrate React-Polymorph render props in Daedalus [DDW-245] Integrate react-polymorph render props in Daedalus Jul 10, 2018
@nikolaglumac
Copy link
Contributor

@DominikGuzei @MarcusHurney checkbox disabled state styles fixed!

screen shot 2018-07-10 at 15 08 30
screen shot 2018-07-10 at 15 08 49
screen shot 2018-07-10 at 15 09 17

@DominikGuzei
Copy link
Member

@nikolaglumac @MarcusHurney this PR is ready for final review :)

@DominikGuzei DominikGuzei removed the WIP label Jul 11, 2018
nikolaglumac
nikolaglumac previously approved these changes Jul 12, 2018
View reviewed changes
Copy link
Contributor

@nikolaglumac nikolaglumac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work @DominikGuzei @MarcusHurney 🎉
Everything is now working perfectly! 👍
Let's merge and release React-Polymoprh version and then update the package.json in this PR. After that it is ready to be merged!

nikolaglumac
nikolaglumac previously approved these changes Jul 12, 2018
View reviewed changes
@DominikGuzei DominikGuzei merged commit 4ad058e into develop Jul 23, 2018
@DominikGuzei DominikGuzei deleted the chore/ddw-245-integrate-react-polymorph-render-props-in-Daedalus branch July 23, 2018 13:27
This was referenced Oct 16, 2018
Closed
Merged
@SebastienGllmt SebastienGllmt mentioned this pull request Dec 28, 2018
Merged
2 tasks
SebastienGllmt added a commit to Emurgo/yoroi-frontend that referenced this pull request Jan 15, 2019
@SebastienGllmt
Upgrade to React-Polymorph v8 (#201)
1b93d9f 
To upgrade to the latest version of `react-polymorph` (v8) we need to do two upgrades:

- [x] upgrade to v7.2 (based on [this PR](input-output-hk/daedalus#950))
- [x] upgrade to v8 (based on [this PR](input-output-hk/daedalus#1139))

I made both upgrades as the two commits to this PR and it's probably easier to read if you code review the two commits individually (since the changes are orthogonal). I also have a README change that explains what `react-polymorph` v8 does but you can find something similar in the Daedalus PR I linked above.

v8 also introduces large refactoring changes that makes a mess of the whole thing. I tried my best to make all the UI look the same while keeping the improvements from v8 but one thing I couldn't stop is that the font size is not 1px bigger across the app (which honestly is not a big deal so I didn't bother). There are probably small things that changed here in there but I manually compared the whole UI and everything looked close enough I don't think anybody would notice any difference that did occur.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikolaglumac nikolaglumac nikolaglumac left review comments

@DominikGuzei DominikGuzei DominikGuzei left review comments

Assignees

@MarcusHurney MarcusHurney

Labels
chore release-0.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarcusHurney @nikolaglumac @DominikGuzei